-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alzollin/markdown features #588
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these changes seem pretty good. My only concern is that we're routing the event for handling clicks via the config object, which makes it more difficult to set from XAML.
That is a fair point, but isn't that true for any property on the config? I did it this way since it wasn't that straightforward to add it to the markdowntextblock itself, since we have no easy reference to it. |
True, and anything on the config could be turned into a separate dependency property if we wanted binding support for the individual options. The distinction seems to be that the config options (custom svg renderer, image provider) are a bit more advanced than a |
This is also the behavior from the 7.x version of our |
You are right. Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This will see plenty of usage and brings us closer to feature parity with the 7.x control. Let's ship it 🚀
Nice set of changes! Are we merging this soon? |
@azchohfi would you have a chance to rebase this PR? Then we can merge: I tried locally, but there are conflicting changes with your other PR #586 (at least that's what I thought the merge view said, but then I don't see the changed file) that I'm not 100% confident in resolving in MyHyperlinkButton.cs within the constructor. |
Ah I see the conflict is from #587 - that maybe helps as that is simple. I'll see if I can rebase this myself again. |
7e039e3
to
02d27e4
Compare
Alright, all fixed! Set to auto-merge once the CI passes, thanks folks! |
No description provided.